Skip to content

fix: catch and rethrow SSEError during SSE connection establishment #975

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Sep 30, 2025
Merged

fix: catch and rethrow SSEError during SSE connection establishment #975

merged 2 commits into from
Sep 30, 2025

Conversation

zhangch-ss
Copy link
Contributor

Motivation and Context

Some errors during SSE connection (e.g., SSEError) are fatal and render read/write streams unusable. Instead of sending the exception via read_stream_writer, which may hang, the error is now raised directly so that the caller can handle it appropriately.

How Has This Been Tested?

mcp-config

{
    "mcpServers": {
        "zhipu-web-search-sse": {
            "url": "https://open.bigmodel.cn/api/mcp/web_search/sse?Authorization=xxxxx"
        }
    }
}

Before fix
Error in sse_reader: Expected response header Content-Type to contain 'text/event-stream', got 'application/json'

After fix

    | Traceback (most recent call last):
    |   File "/app/app/core/sse_client.py", line 152, in sse_client
    |     endpoint_url = await tg.start(sse_reader)
    |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^
    |   File "/usr/local/lib/python3.12/site-packages/anyio/_backends/_asyncio.py", line 898, in start
    |     return await future
    |            ^^^^^^^^^^^^
    |   File "/app/app/core/sse_client.py", line 119, in sse_reader
    |     raise sse_exc
    |   File "/app/app/core/sse_client.py", line 71, in sse_reader
    |     async for sse in event_source.aiter_sse():
    |   File "/usr/local/lib/python3.12/site-packages/httpx_sse/_api.py", line 37, in aiter_sse
    |     self._check_content_type()
    |   File "/usr/local/lib/python3.12/site-packages/httpx_sse/_api.py", line 18, in _check_content_type
    |     raise SSEError(
    | httpx_sse._exceptions.SSEError: Expected response header Content-Type to contain 'text/event-stream', got 'application/json'

Breaking Changes

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

felixweinberger
felixweinberger requested changes Sep 17, 2025
View reviewed changes
Copy link
Contributor

@felixweinberger felixweinberger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @zhangch-ss thanks for this contribution!

Catching more specific errors seems an uncontroversial improvement to me so happy to accept this if we use logger.exception(...) instead which is designed for this.

src/mcp/client/sse.py Outdated
case _:
logger.warning(f"Unknown SSE event: {sse.event}")
except SSEError as sse_exc:
logger.error(f"SSE error: {sse_exc}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use logger.exception which is designed for this purpose? I.e.

logger.exception("Encountered SSE Exception")

The logger automatically provides the exception information.

@felixweinberger felixweinberger added the needs more work Not ready to be merged yet, needs additional changes. label Sep 17, 2025
@felixweinberger felixweinberger added needs maintainer action Potentially serious issue - needs proactive fix and maintainer attention and removed needs more work Not ready to be merged yet, needs additional changes. labels Sep 30, 2025
@felixweinberger felixweinberger self-assigned this Sep 30, 2025
@felixweinberger
Use logger.exception for SSE error logging
b34f51c 
The logger.exception method is specifically designed for logging
exceptions and automatically includes the exception traceback,
providing better debugging information than logger.error.
@felixweinberger felixweinberger added bug Something isn't working and removed needs maintainer action Potentially serious issue - needs proactive fix and maintainer attention labels Sep 30, 2025
@felixweinberger felixweinberger merged commit f676f6c into modelcontextprotocol:main Sep 30, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixweinberger felixweinberger felixweinberger approved these changes

Assignees

@felixweinberger felixweinberger

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@zhangch-ss @felixweinberger